-
Notifications
You must be signed in to change notification settings - Fork 8.2k
bluetooth: Fix L2CAP CoC response code if LTK is present #40177
New issue
Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.
By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.
Already on GitHub? Sign in to your account
Conversation
subsys/bluetooth/host/l2cap.c
Outdated
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am not sure I follow this statement.
If e.g. conn->sec_level is BT_SECURITY_L3 and server->sec_level is BT_SECURITY_L4 this will return with BT_L2CAP_LE_ERR_AUTHENTICATION, but couldn't it just as likely fail with BT_L2CAP_LE_ERR_KEY_SIZE?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
If I get the code correctly, key size is verified by server user in accept callback
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
But then that will leave this function to assume that a specific security check is done elsewhere. It's just a matter of responding with the right error code, so it's not a huge deal, but I do think that this check could possibly (should?) be expanded to check the key in the specific case that if current security is L3 and required security is L4, that we check if the the key size is correct or not.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
if you are referring to L4 requiring 16 key size than this is check by update_sec_level(), ie connection security will not be set to L4 if keysize is not max
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I am referring to that, but in the case where the ACL connection is only L3 and the L2CAP connection requires L4.
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving check from application provided accept context into this check could have potentially affect application behavior (and thus API). This is going to be backported into LTS branch and aims at fixing qualification, not reworking L2CAP API.
I'm worried we may be talking past each other. If the application supplies BT_SECURITY_L4 as the required security, why shouldn't that be checked in the same way that BT_SECURITY_L3 is?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
that said, I'm not sure why it was designed that way, and I'm fine with moving check into stack, just in separate PR
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Moving check from application provided accept context into this check could have potentially affect application behavior (and thus API). This is going to be backported into LTS branch and aims at fixing qualification, not reworking L2CAP API.
I'm worried we may be talking past each other. If the application supplies
BT_SECURITY_L4as the required security, why shouldn't that be checked in the same way thatBT_SECURITY_L3is?
but it is checked the same way (or maybe I'm misunderstanding what is the problem here)
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
Adding that in a separate PR would be fine with me, but could you please add a TODO here so that we don't completely forget it?
There was a problem hiding this comment.
Choose a reason for hiding this comment
The reason will be displayed to describe this comment to others. Learn more.
I've created issue for this #40216
If an LTK or an STK is available and encryption is required (LE security mode 1) but encryption is not enabled, the service request shall be rejected with the error code "Insufficient Encryption". This is affecting L2CAP/LE/CFC/BV-25-C and L2CAP/ECFC/BV-32-C qualification test cases. Signed-off-by: Szymon Janc <[email protected]>
e6f10fc to
0d42b47
Compare
If an LTK or an STK is available and encryption is required
(LE security mode 1) but encryption is not enabled, the
service request shall be rejected with the error code
"Insufficient Encryption".
This is affecting L2CAP/LE/CFC/BV-25-C and L2CAP/ECFC/BV-32-C
qualification test cases.
Signed-off-by: Szymon Janc [email protected]